Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed minimal cuts to allow parsing #833 #834

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

marcoeilers
Copy link
Contributor

No description provided.

@jcp19 jcp19 requested a review from JonasAlaif January 16, 2025 11:18
@jcp19
Copy link
Contributor

jcp19 commented Feb 3, 2025

can we go ahead and merge this?

@marcoeilers
Copy link
Contributor Author

@JonasAlaif It would be great if you could have a look at this. It's a tiny change obviously but maybe there are some principles to when and how you used cuts that I'm breaking.

@JonasAlaif
Copy link
Contributor

Sorry for not looking at this earlier. What is this meant to address? The cuts help improve performance and parse error messages so we should try to keep them in if possible.

@marcoeilers
Copy link
Contributor Author

The code in #833 doesn't parse with the cuts bc it thinks there might be an annotated expression after decreases, but it's an annotated function, but it cannot backtrack bc of the cut in annotation parsing.

@JonasAlaif
Copy link
Contributor

I see. Why in the jeebus do we allow such ambiguous syntax... If this is really what we want, I would just special case the expr annotation immediately after a decreases to not have a cut (i.e. decreases ~ specialExp) and avoid removing the good cuts elsewhere.

@marcoeilers
Copy link
Contributor Author

This isn't what we want, this is unfortunately what we already have. I'll try doing that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants